Repository navigation
Conversation
proxyRequest stages the backend's status and headers before forwarding the body. When a backend sends a complete header block and closes before sending the declared body, ForwardBody returns an error without writing anything, so the retry loop continues on another pod. The failed attempt's headers are still staged, and copyResponseHeaders appends the healthy attempt's values. The client then receives two Content-Length headers plus stale cookies and headers from the failed backend. Snapshot the headers and status that earlier handlers staged, and restore them when forwarding fails before anything is committed. Headers added by router middleware survive, and the healthy retry carries one Content-Length and only its own upstream headers. TestProxy_RetryDropsFailedAttemptHeaders drives the production path with a real backend that declares 4096 bytes and closes, followed by a healthy retry. Signed-off-by: Saylee <saylee21@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues block the change to prevent failed backend attempts from leaking headers into successful retries.
No open issues 🎉
What changed in this PR
Fixes #1936 by preventing failed backend attempts from leaking response headers into successful retries in the router.
Changes:
- Snapshots staged headers and status before forwarding, restoring them on failure only if the downstream response has not been committed.
- Adds a regression test covering retry header cleanup, preservation of middleware headers, and successful fallback.
| File | Description |
|---|---|
pkg/kthena-router/router/router.go |
Restores pre-attempt headers and status after an uncommitted forwarding failure. |
pkg/kthena-router/router/router_test.go |
Tests that a healthy retry excludes failed-backend headers while preserving middleware headers. |
Review effort: Balanced
| // The caller may retry another backend while nothing has been written | ||
| // downstream. Snapshot what earlier handlers staged so headers copied from a | ||
| // backend that fails before sending its body cannot leak into the retry. | ||
| stagedHeaders := cloneResponseHeaders(c.Writer.Header()) | ||
| stagedStatus := c.Writer.Status() | ||
| forwardErr := forwardResponseWithUsageParser(c, resp, stream, parser, onUsage) | ||
| if forwardErr != nil && !c.Writer.Written() { | ||
| restoreResponseHeaders(c.Writer.Header(), stagedHeaders) | ||
| c.Status(stagedStatus) | ||
| } |
There was a problem hiding this comment.
The PD-disaggregated retry loop has the same problem and isn't covered by this fix. decoderProxy also calls copyResponseHeaders (Header().Add) and c.Status before forwarding the body (transport.go#L146-L147). On the non-streaming path, io.Copy returns before writing anything when the first read fails. So proxyToPDDisaggregated sees !c.Writer.Written() and moves on to the next prefill/decode pair with the failed attempt's headers still staged (router.go#L1601-L1621).
I checked this with the same fixture (200, Content-Length: 4096, then close) against decoderProxy. It returns non-streaming decode interrupted: unexpected EOF with Written() == false, and Content-Length: 4096 / X-Backend: failed left in c.Writer.Header(). If every pair fails, the final 500 error response also carries the stale Content-Length.
Could the snapshot/restore be pulled into a small helper and applied around kvConnector.Proxy in the PD loop as well? That would cover all connectors at once instead of fixing each one.
What type of PR is this?
/kind bug
What this PR does / why we need it:
proxyRequeststages the backend's status and headers before forwarding the body. When a backend sends a complete header block and then closes before sending the declared body,providers.ForwardBodyreturns an error before writing anything, soproxyretries on the next pod. The failed attempt's headers are still staged, andcopyResponseHeadersappends the healthy attempt's values withHeader().Add. The client receives conflictingContent-Lengthheaders and stale cookies and upstream headers from the backend that failed.This snapshots the headers and status that earlier handlers staged, and restores them when forwarding fails before anything is committed. Headers added by router middleware survive, and the healthy retry carries a single
Content-Lengthand only its own upstream headers. Nothing is restored once a byte has been written, so a partial response still ends the request.Which issue(s) this PR fixes:
Fixes #1936
Bug evidence (required for bug-related PRs):
The regression test drives the production path: a real aggregated ModelServer with two pods, a real HTTP backend on the other side, and
router.HandlerFunc()as the entry point. The first backend hijacks the connection and writesHTTP/1.1 200 OKwithContent-Length: 4096,Set-Cookie: failed-backend=1andX-Backend: failed, then closes without a body. The retry backend returnsContent-Length: 16and{"id":"healthy"}.Without the fix the test fails on the recorder:
With the fix it passes: the recorder has one
Content-Length: 16,X-Backend: healthy, noSet-Cookie, the middleware header is preserved, and the body is{"id":"healthy"}. The test also checks that exactly two attempts reached the backend.Environment: macOS, Go 1.27.2,
go test -race ./pkg/kthena-router/router/ -count=1. The reporter of #1936 reproduced the same header output against the unmodified standalone binary.Production paths involved:
pkg/kthena-router/router/router.go:proxyRequest,copyResponseHeaderspkg/kthena-router/providers/stream.go:ForwardBodySpecial notes for your reviewer:
proxyRequestis only called from the retry loop inproxy, so the snapshot is limited to paths that may retry. Streaming responses keep their existing behavior: the restore branch only runs whenc.Writer.Written()is false.Does this PR introduce a user-facing change?: